Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

auto/otel: Fix breakage on non-Linux platforms #1520

Closed
wants to merge 1 commit into from

Conversation

ac000
Copy link
Member

@ac000 ac000 commented Dec 20, 2024

auto/otel: Fix breakage on non-Linux platforms

When building with --otel on macOS for example I was seeing compile
failures with the cpu_set_t stuff which should only be used under Linux.

It turned out that despite

  checking for Linux sched_getaffinity() ... not found

we were getting

  #ifndef NXT_HAVE_LINUX_SCHED_GETAFFINITY
  #define NXT_HAVE_LINUX_SCHED_GETAFFINITY  1
  #endif

in build/include/nxt_auto_config.h

It seems this was due to the

    . auto/feature

in auto/otel, this check happens right after the above. Without having

    nxt_feature_name=NXT_HAVE_OTEL

set.

Instead we were adding the define for that manually.

Doing auto/feature without having a nxt_feature_name must have used the
last set one and enabled it.

Set nxt_feature_name and remove the manual editing of nxt_auto_config.h

Signed-off-by: Andrew Clayton <[email protected]>

When building with --otel on macOS for example I was seeing compile
failures with the cpu_set_t stuff which should only be used under Linux.

It turned out that despite

  checking for Linux sched_getaffinity() ... not found

we were getting

  #ifndef NXT_HAVE_LINUX_SCHED_GETAFFINITY
  #define NXT_HAVE_LINUX_SCHED_GETAFFINITY  1
  #endif

in build/include/nxt_auto_config.h

It seems this was due to the

    . auto/feature

in auto/otel, this check happens right after the above. Without having

    nxt_feature_name=NXT_HAVE_OTEL

set.

Instead we were adding the define for that manually.

Doing auto/feature without having a nxt_feature_name must have used the
last set one and enabled it.

Set nxt_feature_name and remove the manual editing of nxt_auto_config.h

Signed-off-by: Andrew Clayton <[email protected]>
@ac000 ac000 marked this pull request as ready for review December 20, 2024 00:09
@ac000 ac000 requested review from thresheek and avahahn December 20, 2024 00:09
@thresheek
Copy link
Member

This patch seems correct to address this particular issue.

My fix for macOS build uses a similar trick, but we also need to link with additional frameworks on that OS: thresheek@660e853

Also, another fix is required to be able to build on macOS: thresheek@18381b1

We probably want to incorporate those changes in this PR for the sake of completeness.

@ac000
Copy link
Member Author

ac000 commented Dec 20, 2024

This patch seems correct to address this particular issue.

It would affect any system that didn't have a Linux compatible sched_getaffinity(2), not that I was aware of any that did, but it seems at least FreeBSD does, so I'll need to tweak the commit subject...

My fix for macOS build uses a similar trick, but we also need to link with additional frameworks on that OS: thresheek@660e853

Weird, I wonder what is doing that... it certainly uses regular OpenSSL for general TLS support...

Also, another fix is required to be able to build on macOS: thresheek@18381b1

This one makes sense.

We probably want to incorporate those changes in this PR for the sake of completeness.

But then there is some other breakage also with --otel

(in auto/make)

 87 ifeq (\$D,1)
 88         NXT_OTEL_LIB_LOC = $NXT_OTEL_LIB_DIR/target/debug/libotel.a
 89 else
 90         NXT_OTEL_LIB_LOC = $NXT_OTEL_LIB_DIR/target/release/libotel.a
 91 endif

Prevents the use of make on at least FreeBSD, however, it also breaks with gmake as D has not been set as that only happens when

 44 if [ -n "$NXT_GNU_MAKE" ] || [ $NXT_OS = "SunOS" ]; then

And then above that we have

 64 ifeq (\$D,1)                                                                
 65         CFLAGS += -O0                                                       
 66         RUST_FLAGS += --debug                                               
 67 else                                                                        
 68         RUST_FLAGS += --release                                             
 69 endif

However that is guarded by the

 44 if [ -n "$NXT_GNU_MAKE" ] || [ $NXT_OS = "SunOS" ]; then                 

So on the likes of FreeBSD RUST_FLAGS isn't set all...

I don't mind making GNU make a build dependency... however at the moment it seems --otel is fubar on anything non-Linux... (between needing sched_getaffinity(2) and GNU make being the default make(1))

@ac000 ac000 marked this pull request as draft December 20, 2024 04:31
@ac000 ac000 mentioned this pull request Dec 20, 2024
@ac000
Copy link
Member Author

ac000 commented Dec 20, 2024

Some of this would be simpler if we could just tell rustc to write the library to build/lib/libotel.a, oh right, we can...

cargo build ... --emit link=../../build/lib/libotel.a

and then just do debug builds by default and enable release builds by setting a cargo environment variable, but that seems to be a stumbling block as unless I missed it, that seems to be the one thing you can't set...

@ac000
Copy link
Member Author

ac000 commented Dec 21, 2024

While we're at it could we

s/NXT_OTEL_LIB_LOC/NXT_OTEL_LIB_STATIC

ac000 added a commit to ac000/unit that referenced this pull request Jan 6, 2025
There is breakage on non-GNU make systems due to the OTEL stuff using
'ifeq'.

We had previously handled this by only using such constructs (and
enabling certain features, such as D= & E=) when using GNU make.

OTEL added this unconditionally. It's used to set the target build
directory appropriately depending on whether we are doing debug or
release builds.

Unfortunately it seems there is no way to control this via cargo/rustc
command line arguments or environment variables.

For BSD make we could use '.if' etc but then trying to handle these two
systems (and possibly more) in the build scripts is going to be
unwieldy.

Let's just bite the bullet and make (no pun intended) GNU make mandatory
to build Unit, it's the default on Linux (I'm sure there's an exception
somewhere...) and it seems the default on macOS. On other systems it's
readily available via gmake(1).

This also brings build feature parity to all systems out the box.

As it happens, this also fixes issues with using gmake...

Fixes: 9d3dcb8 ("otel: add build tooling to include otel code")
Link: <nginx#1520 (comment)>
Signed-off-by: Andrew Clayton <[email protected]>
ac000 added a commit to ac000/unit that referenced this pull request Jan 7, 2025
There is breakage on non-GNU make systems due to the OTEL stuff using
'ifeq'.

We had previously handled this by only using such constructs (and
enabling certain features, such as D= & E=) when using GNU make.

OTEL added this unconditionally. It's used to set the target build
directory appropriately depending on whether we are doing debug or
release builds.

Unfortunately it seems there is no way to control this via cargo/rustc
command line arguments or environment variables.

For BSD make we could use '.if' etc but then trying to handle these two
systems (and possibly more) in the build scripts is going to be
unwieldy.

Let's just bite the bullet and make (no pun intended) GNU make mandatory
to build Unit, it's the default on Linux (I'm sure there's an exception
somewhere...) and it seems the default on macOS. On other systems it's
readily available via gmake(1).

This also brings build feature parity to all systems out the box.

As it happens, this also fixes issues with using gmake...

Fixes: 9d3dcb8 ("otel: add build tooling to include otel code")
Link: <nginx#1520 (comment)>
Signed-off-by: Andrew Clayton <[email protected]>
ac000 added a commit to ac000/unit that referenced this pull request Jan 7, 2025
There is breakage on non-GNU make systems due to the OTEL stuff using
'ifeq'.

We had previously handled this by only using such constructs (and
enabling certain features, such as D= & E=) when using GNU make.

OTEL added this unconditionally. It's used to set the target build
directory appropriately depending on whether we are doing debug or
release builds.

Unfortunately it seems there is no way to control this via cargo/rustc
command line arguments or environment variables.

For BSD make we could use '.if' etc but then trying to handle these two
systems (and possibly more) in the build scripts is going to be
unwieldy.

Let's just bite the bullet and make (no pun intended) GNU make mandatory
to build Unit, it's the default on Linux (I'm sure there's an exception
somewhere...) and it seems the default on macOS. On other systems it's
readily available via gmake(1).

This also brings build feature parity to all systems out the box.

As it happens, this also fixes issues with using gmake...

Fixes: 9d3dcb8 ("otel: add build tooling to include otel code")
Link: <nginx#1520 (comment)>
Signed-off-by: Andrew Clayton <[email protected]>
@ac000
Copy link
Member Author

ac000 commented Jan 7, 2025

Closing this PR as this is now a part of #1527

@ac000 ac000 closed this Jan 7, 2025
@ac000 ac000 mentioned this pull request Jan 7, 2025
ac000 added a commit to ac000/unit that referenced this pull request Jan 7, 2025
There is breakage on non-GNU make systems due to the OTEL stuff using
'ifeq'.

We had previously handled this by only using such constructs (and
enabling certain features, such as D= & E=) when using GNU make.

OTEL added this unconditionally. It's used to set the target build
directory appropriately depending on whether we are doing debug or
release builds.

Unfortunately it seems there is no way to control this via cargo/rustc
command line arguments or environment variables.

For BSD make we could use '.if' etc but then trying to handle these two
systems (and possibly more) in the build scripts is going to be
unwieldy.

Let's just bite the bullet and make (no pun intended) GNU make mandatory
to build Unit, it's the default on Linux (I'm sure there's an exception
somewhere...) and it seems the default on macOS. On other systems it's
readily available via gmake(1).

This also brings build feature parity to all systems out the box.

As it happens, this also fixes issues with using gmake...

Fixes: 9d3dcb8 ("otel: add build tooling to include otel code")
Link: <nginx#1520 (comment)>
Signed-off-by: Andrew Clayton <[email protected]>
ac000 added a commit to ac000/unit that referenced this pull request Jan 9, 2025
There were at least a couple of issues with building OTEL support.

It only worked with GNU make due to the use of ifeq, even gmake had some
issues.

Debug builds were broken due to trying to pass --debug to cargo which is
the default and isn't a valid option.

This 'fixes' things by doing 'release' builds of OTEL by default.

Passing D=1 to make will generate 'debug' builds but this as previously
with D= etc, only works with GNU make.

Fixes: 9d3dcb8 ("otel: add build tooling to include otel code")
Link: <nginx#1520 (comment)>
Signed-off-by: Andrew Clayton <[email protected]>
ac000 added a commit to ac000/unit that referenced this pull request Jan 9, 2025
There were at least a couple of issues with building OTEL support.

It only worked with GNU make due to the use of ifeq, even gmake had some
issues.

Debug builds were broken due to trying to pass --debug to cargo which is
the default and isn't a valid option.

This 'fixes' things by doing 'release' builds of OTEL by default.

Passing D=1 to make will generate 'debug' builds but this as previously
with D= etc, only works with GNU make.

We make use the of the '--emit link=' rustc option to place the
libotel.a static library into build/lib

This is good, it consolidates the static libraries into one place and it
simplifies the build scripts.

While we're at it pretty print the cargo command by default.

Fixes: 9d3dcb8 ("otel: add build tooling to include otel code")
Link: <nginx#1520 (comment)>
Signed-off-by: Andrew Clayton <[email protected]>
ac000 added a commit to ac000/unit that referenced this pull request Jan 9, 2025
There were at least a couple of issues with building OTEL support.

It only worked with GNU make due to the use of ifeq, even gmake had some
issues.

Debug builds were broken due to trying to pass --debug to cargo which is
the default and isn't a valid option.

This 'fixes' things by doing 'release' builds of OTEL by default.

Passing D=1 to make will generate 'debug' builds but this as previously
with D= etc, only works with GNU make.

We make use of the '--emit link=' rustc option to place the libotel.a
static library into build/lib

This is good, it consolidates the static libraries into one place and it
simplifies the build scripts.

While we're at it pretty print the cargo command by default.

Fixes: 9d3dcb8 ("otel: add build tooling to include otel code")
Link: <nginx#1520 (comment)>
Signed-off-by: Andrew Clayton <[email protected]>
ac000 added a commit to ac000/unit that referenced this pull request Jan 10, 2025
There were at least a couple of issues with building OTEL support.

It only worked with GNU make due to the use of ifeq, even gmake had some
issues.

Debug builds were broken due to trying to pass --debug to cargo which is
the default and isn't a valid option.

This 'fixes' things by doing 'release' builds of OTEL by default.

Passing D=1 to make will generate 'debug' builds but this as previously
with D= etc, only works with GNU make.

We make use of the '--emit link=' rustc option to place the libotel.a
static library into build/lib

This is good, it consolidates the static libraries into one place and it
simplifies the build scripts.

While we're at it pretty print the cargo command by default.

Fixes: 9d3dcb8 ("otel: add build tooling to include otel code")
Link: <nginx#1520 (comment)>
Signed-off-by: Andrew Clayton <[email protected]>
ac000 added a commit to ac000/unit that referenced this pull request Jan 10, 2025
There were at least a couple of issues with building OTEL support.

It only worked with GNU make due to the use of ifeq, even gmake had some
issues.

Debug builds were broken due to trying to pass --debug to cargo which is
the default and isn't a valid option.

This 'fixes' things by doing 'release' builds of OTEL by default.

Passing D=1 to make will generate 'debug' builds but this as previously
with D= etc, only works with GNU make.

We make use of the '--emit link=' rustc option to place the libotel.a
static library into build/lib

This is good, it consolidates the static libraries into one place and it
simplifies the build scripts.

While we're at it pretty print the cargo command by default.

Fixes: 9d3dcb8 ("otel: add build tooling to include otel code")
Link: <nginx#1520 (comment)>
Signed-off-by: Andrew Clayton <[email protected]>
thresheek pushed a commit that referenced this pull request Jan 10, 2025
There were at least a couple of issues with building OTEL support.

It only worked with GNU make due to the use of ifeq, even gmake had some
issues.

Debug builds were broken due to trying to pass --debug to cargo which is
the default and isn't a valid option.

This 'fixes' things by doing 'release' builds of OTEL by default.

Passing D=1 to make will generate 'debug' builds but this as previously
with D= etc, only works with GNU make.

We make use of the '--emit link=' rustc option to place the libotel.a
static library into build/lib

This is good, it consolidates the static libraries into one place and it
simplifies the build scripts.

While we're at it pretty print the cargo command by default.

Fixes: 9d3dcb8 ("otel: add build tooling to include otel code")
Link: <#1520 (comment)>
Signed-off-by: Andrew Clayton <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants